Apply DNS host config on change only #4695
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes DNS host configuration updates by applying changes only when the configuration has actually changed, preventing unnecessary updates when the config remains identical.
Key Changes:
- Added hash-based change detection to compare current and new DNS host configurations
- Introduced
currentConfigHashfield to track the hash of the applied configuration
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/internal/dns/server.go
Outdated
| } | ||
|
|
||
| if s.currentConfigHash == hash { | ||
| log.Infof("not applying host config as there are no changes") |
There was a problem hiding this comment.
[nitpick] Changed 'Infof' to 'Debugf' for consistency with logging levels - configuration skip events are typically debug-level information rather than info-level.
| log.Infof("not applying host config as there are no changes") | |
| log.Debugf("not applying host config as there are no changes") |
hurricanehrndz
force-pushed
the
apply_dns_hostconfig_on_change_only
branch
2 times, most recently
from
a0ba525 to
7bd88f1
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds per-instance hash caching for merged host DNS configuration by introducing a Changes
Sequence DiagramsequenceDiagram
participant Caller
participant DefaultServer
participant Hasher as Hash
Caller->>DefaultServer: Apply host DNS config
DefaultServer->>Hasher: Compute hash(merged config + extra domains)
alt Hash computation fails
Hasher-->>DefaultServer: Error
DefaultServer->>DefaultServer: Proceed to apply (fail-safe)
DefaultServer-->>Caller: Return (applied despite hash error)
else Hash computed
Hasher-->>DefaultServer: hashValue
alt hashValue == currentConfigHash
DefaultServer->>DefaultServer: Skip applying config
DefaultServer-->>Caller: Return (no-op)
else hashValue != currentConfigHash
DefaultServer->>DefaultServer: Apply new config
DefaultServer->>DefaultServer: Set currentConfigHash = hashValue
DefaultServer-->>Caller: Return (updated)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Initialize currentConfigHash to max uint64 to ensure first config application, implement fail-safe error handling for hash computation failures, and only update hash after successful config application to maintain consistency
hurricanehrndz
force-pushed
the
apply_dns_hostconfig_on_change_only
branch
from
7bd88f1 to
2d29138
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/internal/dns/server.go (1)
591-613: Well-implemented hash-based change detection with proper fail-safe behavior.The logic correctly:
- Computes hash of the merged config (including extra domains)
- Applies a fail-safe approach by proceeding with application if hashing fails
- Skips application only when hashing succeeds and the hash matches
- Updates
currentConfigHashonly when both hashing and application succeedThe fail-safe handling at lines 598-599 ensures that transient hash computation failures won't prevent necessary configuration updates, which is the right trade-off between optimization and reliability.
Minor: Comment clarity (optional refinement)
The comment at line 610 is accurate, but could be more explicit. Consider:// Only update hash if both operations succeeded: // - Hash computation succeeded (err == nil from line 591) // - Config application succeeded (would have returned at line 607 otherwise)This makes it clearer why checking
err == nilalone is sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/internal/dns/server.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/internal/dns/server.go (2)
client/internal/dns/host.go (1)
applyDNSConfig(11-16)client/internal/dns/host_darwin.go (1)
s(50-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Client / Unit
🔇 Additional comments (2)
client/internal/dns/server.go (2)
83-83: LGTM: New field added to track config state.The
currentConfigHashfield is appropriately placed and will enable efficient change detection.
211-211: Excellent initialization strategy.Using
^uint64(0)(max uint64) as the sentinel value is a robust approach that ensures the first configuration is always applied while minimizing collision risk with actual hash values.
1 participant
Describe your changes
Host config should only be applied when it has changed
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit